USHIFT-6795: C2CC: Router controller#6599
Open
pmtk wants to merge 36 commits intoopenshift:mainfrom
Open
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pmtk: This pull request references USHIFT-6795 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Cluster-to-Cluster (C2CC) route manager to MicroShift that implements OVN NB route management, Linux policy/service routing, nftables bypass rules, node SNAT annotation management, a C2CC NetworkPolicy, service registration, unit tests, and Robot Framework end-to-end scenarios. ChangesC2CC Route Manager (single cohesive change DAG)
Sequence Diagram(s)sequenceDiagram
participant MS as MicroShift
participant CM as C2CCRouteManager
participant OVN as OVN_NB
participant K8s as Kubernetes_API
participant NL as Linux_Netlink
participant NFT as nftables
MS->>CM: Start manager (NewC2CCRouteManager / Run)
CM->>OVN: Connect & Monitor NB (unix socket, backoff)
CM->>K8s: Init kube client
CM->>NL: Subscribe routes/rules events
CM->>NFT: Subscribe nft events
CM->>CM: Start reconcile loop (periodic + event-driven)
alt On reconcile (tick or event)
CM->>OVN: List/Create/Delete Logical_Router_Static_Route
CM->>K8s: Get/Patch Node annotations, Reconcile NetworkPolicy
CM->>NL: Reconcile kernel policy routes and ip rules
CM->>NFT: Reconcile nftables bypass rules (add/remove)
OVN-->>CM: state / events
K8s-->>CM: state / events
NL-->>CM: state / events
NFT-->>CM: state / events
end
MS->>CM: Shutdown
CM->>OVN: Cleanup routes
CM->>NL: Cleanup routes/rules
CM->>NFT: Cleanup nft rules
CM->>K8s: Delete NetworkPolicy / clear annotations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/apparentlymart/go-cidr@v1.1.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/go-systemd@v0.0.0-20190321100706-95778dfbb74e: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/google/go-cmp@v0.7.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/miekg/dns@v1.1.63: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/openshift/api@v0.0.0-20260309155933-45fd88d185dd: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/openshift/build-machinery-go@v0.0.0-20251023084048-5d77c1a5e5af: is explicitly required in go.mod, but not marked as explicit ... [truncated 30811 characters] ... elet: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/metrics: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/mount-utils: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/pod-security-admission: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-cli-plugin: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-controller: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pmtk The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (4)
pkg/controllers/c2cc/nbdb_test.go (1)
16-41:FieldTagstests are currently tautological.These assertions only re-check values set in the same struct literal, so they won’t catch
ovsdbtag/schema regressions. Please validate tag mappings directly (e.g., reflection on struct tags or model metadata checks).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/c2cc/nbdb_test.go` around lines 16 - 41, The tests TestLogicalRouterStaticRoute_FieldTags and TestLogicalRouter_FieldTags are tautological because they only re-assert values set in struct literals; replace these with reflection-based assertions that verify the actual struct field tags (e.g., the ovsdb/model tags) on the types instead of field values. Use reflect.TypeOf(LogicalRouterStaticRoute{}) and reflect.TypeOf(LogicalRouter{}) to inspect fields UUID, IPPrefix, Nexthop, ExternalIDs, Policy and UUID, Name, StaticRoutes respectively, and assert that Tag.Get("ovsdb") (or the correct tag key used by your OVSDB mapping) equals the expected tag/schema name for each field; update the test expectations to the correct tag strings so the tests fail on tag/schema regressions. Ensure you keep the original test names and replace value assertions with these tag checks.test/suites/c2cc/reconciliation.robot (1)
19-21: 30s timeout may be tight for some reconciliation scenarios.The controller's periodic reconcile is 10s (
reconcileInterval). With 5s retry, you get ~6 attempts. Should be sufficient, but event-driven reconciliation should trigger faster.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/c2cc/reconciliation.robot` around lines 19 - 21, The test's reconciliation timeout (${RECONCILE_TIMEOUT}) is likely too short for slower environments; increase ${RECONCILE_TIMEOUT} (or adjust ${RECONCILE_RETRY}) so the suite allows more attempts given the controller's reconcileInterval (~10s). Update the Variables block to set a larger ${RECONCILE_TIMEOUT} (for example to 60s or more) or reduce ${RECONCILE_RETRY} gap to ensure enough reconciliation attempts relative to reconcileInterval and event-driven triggers.pkg/controllers/c2cc/ovn.go (1)
25-28: Consider URL-safe encoding for named UUIDs.The replacer handles common characters but
.,/,:,-may not be exhaustive for all CIDR formats. IPv6 CIDRs could introduce other characters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/c2cc/ovn.go` around lines 25 - 28, The current buildNamedUUID uses strings.NewReplacer to sanitize a few characters but misses other characters (e.g., from IPv6 CIDRs); change buildNamedUUID to produce a URL-safe deterministic token by encoding the combined prefix+suffix with a safe encoder: e.g., compute a SHA-256 (or other stable) hash of prefix+suffix and return its URL-safe base64 (base64.RawURLEncoding.EncodeToString(hash[:])) or, if readability is desired, return url.PathEscape(prefix+suffix) to percent-encode all unsafe characters; update the buildNamedUUID function to use one of these approaches so all possible characters are handled safely.test/resources/c2cc.resource (1)
174-180: Python inline script for JSON parsing works but is fragile.Consider using
oc get node -o jsonpathwith the annotation key directly, similar to line 186. The current approach handles edge cases but adds complexity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/resources/c2cc.resource` around lines 174 - 180, Replace the fragile Python inline in the "Verify Node SNAT Annotation" check: instead of piping oc JSON to python3 -c, call Oc On Cluster to run oc get node with a jsonpath that directly extracts the annotation key 'k8s.ovn.org/node-ingress-snat-exclude-subnets' (for example using oc get node -o jsonpath="{.items[0].metadata.annotations['k8s.ovn.org/node-ingress-snat-exclude-subnets']}") so ${stdout} contains the annotation value reliably; update the command that currently uses python3 -c in the test to use this jsonpath form and keep the subsequent Should Contain assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/c2cc/annotation.go`:
- Around line 74-97: The node watch loop currently can leak watches and spin on
reconnects; ensure each watcher is explicitly stopped and that unexpected
channel closure backs off: after successfully creating watcher (from
a.kubeClient.CoreV1().Nodes().Watch with FieldSelector
"metadata.name="+a.nodeName) defer or call watcher.Stop() when you exit that
watch scope, iterate using: ch, ok := <-watcher.ResultChan() and if !ok break
the inner loop and sleep/backoff (e.g., time.Sleep(10*time.Second)) before
continuing, and keep the existing ctx.Done() checks and reconcileCh send
(reconcileCh <- "node-annotation-changed") behavior unchanged so the loop won't
rapidly spin or leak goroutines.
In `@pkg/controllers/c2cc/nbdb.go`:
- Around line 56-58: The current loop around os.Stat(ovnNBSocketPath) treats
every error as "not found" and keeps retrying; update the check so that if
os.Stat returns an error that is not os.IsNotExist(err) you immediately return
that error (or wrap/propagate it) instead of retrying, and only continue retry
logic when os.IsNotExist(err) is true; locate the os.Stat(ovnNBSocketPath) call
in nbdb.go and change the conditional to explicitly handle non-`not exists`
errors (using os.IsNotExist) and return them promptly.
In `@pkg/controllers/c2cc/networkpolicy.go`:
- Around line 27-53: newNetworkPolicyManager currently builds an ingress rule
from ingressPeers which, if remotePodCIDRs is empty, results in an ingress rule
with an empty From (matching all sources); change the logic so you only add the
ingress rule and set networkingv1.PolicyTypeIngress when ingressPeers is
non-empty: compute ingressPeers as before, then conditionally populate
policy.Spec.Ingress and policy.Spec.PolicyTypes only when len(ingressPeers) > 0
(otherwise leave Spec.Ingress nil/empty and omit PolicyTypeIngress) so the
NetworkPolicy does not accidentally allow all ingress; update references to
ingressPeers, newNetworkPolicyManager, policy.Spec.Ingress, and PolicyTypes
accordingly.
- Around line 65-96: The reconcile/update and cleanup paths are mutating a
fixed-named NetworkPolicy without verifying ownership; before
Create/Update/Delete, fetch the existing object (existing from client.Get in the
reconcile flow and the object returned by Delete error handling in cleanup) and
check its labels (specifically "app.kubernetes.io/managed-by") against the
controller's expected manager value (compare
existing.Labels["app.kubernetes.io/managed-by"] to the value used for your
managed resources—e.g., the label on m.desired); if the label exists and does
not match, do not Update or Delete (instead log and return nil or a
non-destructive result), and when creating ensure you set the managed-by label
on m.desired so ownership is explicit; apply this logic inside the
reconcile/update code paths where client.Get, client.Create, client.Update are
used and inside networkPolicyManager.cleanup before deleting the named policy.
In `@pkg/controllers/c2cc/nftables.go`:
- Around line 161-206: The debounce goroutine is leaked because rawCh is never
closed when unsubscribing; change the returned unsubscribe to a closure that
closes rawCh and then calls sock.Close (use a sync.Once to ensure idempotence)
so the receive goroutine unblocks and the debounce goroutine can exit (refer to
rawCh, reconcileCh, the anonymous debounce goroutine, and sock.Close in your
change).
In `@pkg/controllers/c2cc/policy_routes.go`:
- Around line 41-45: The route reconcile code currently only logs
netlink.RouteReplace/RouteDel failures and always returns nil; update
reconcileRoutes to collect and propagate errors so callers
(linuxRouteManager.reconcile and serviceRouteManager.reconcile) see failures:
inside reconcileRoutes (and the block handling netlink.RouteReplace and
netlink.RouteDel) accumulate any failures (e.g., append to a slice or use
multierror) and return a non-nil error if any netlink operation fails, and
adjust callers that expect reconcileRoutes to return error so they propagate
that error instead of treating reconciliation as success.
- Around line 38-45: The equality check that skips RouteReplace only compares
gateways (actual.Gw == route.Gw) but must also compare the interface index;
update the condition that uses actualByDst/actual to require both
actual.Gw.Equal(route.Gw) AND actual.LinkIndex == route.LinkIndex (or
equivalent) before skipping; this ensures stale routes with a changed ifindex
(e.g., ovn-k8s-mp0 recreated) will be replaced by netlink.RouteReplace and
matches how routes are programmed in service_routes.go.
In `@pkg/controllers/c2cc/service_routes.go`:
- Around line 57-72: The loop is using the single IPv4 gateway returned by
getMgmtPortGateway() for every svc CIDR in m.localSvcCIDRs, which breaks
IPv6/dual‑stack; update the logic to pick a gateway that matches the address
family of each svc CIDR (or have getMgmtPortGateway return gateways per family)
before creating netlink.Route entries: for each svcCIDR in m.localSvcCIDRs,
determine whether dst is IPv4 or IPv6 and use the matching gateway IP (or leave
Gw nil if no same‑family gateway exists) when constructing the netlink.Route
(fields Dst, Gw, Table, Protocol, LinkIndex), so only same‑family next hops are
applied.
In `@test/bin/scenario.sh`:
- Line 1242: The local declaration combines with command substitution (local
var_name="${var_prefix}_$(echo "${prop}" | tr '[:lower:]' '[:upper:]')") which
can hide failures under set -e; fix by splitting into two statements: first
declare the variable (local var_name) and then perform the assignment to
var_name using the command substitution (var_name="${var_prefix}_$(echo
"${prop}" | tr '[:lower:]' '[:upper:]')"), referencing the same var_prefix and
prop variables and the var_name identifier so command-substitution failures
correctly propagate.
In `@test/scenarios-bootc/el9/presubmits/el98-src`@c2cc.sh:
- Around line 132-133: The script copies the cluster-admin kubeadmin kubeconfig
to /tmp and makes it world-readable (chmod 644), which exposes credentials;
change the flow so the kubeconfig remains owned by redhat and only readable by
its owner (mode 0600) or avoid the shared temp file entirely. Update the
run_command_on_vm call that creates /tmp/kubeconfig-b to either set ownership
and permissions (chown redhat && chmod 600) instead of chmod 644, or skip
writing to /tmp and have the scp call pull directly from the source path
(/var/lib/microshift/resources/kubeadmin/${host2_ip}/kubeconfig) so you remove
the temporary shared file; reference the run_command_on_vm invocation and the
scp line that uses "${kubeconfig_b}" when making the change.
In `@test/suites/c2cc/reconciliation.robot`:
- Around line 117-123: Replace the non-disruptive lookup with a disruptive
command to avoid a race between discovery and deletion: change the step that
sets ${handle} which currently uses "Command On Cluster" (running `nft list
chain inet ovn-kubernetes ovn-kube-pod-subnet-masq -a | grep
'c2cc-no-masq:${cidr}' | awk '/# handle/{print $NF}'`) to use "Disruptive
Command On Cluster" instead, so the handle discovery and the subsequent
"Disruptive Command On Cluster ... nft delete rule ... handle ${handle}"
both use the disruptive execution path (and consider adding a check for an empty
${handle} before attempting deletion).
---
Nitpick comments:
In `@pkg/controllers/c2cc/nbdb_test.go`:
- Around line 16-41: The tests TestLogicalRouterStaticRoute_FieldTags and
TestLogicalRouter_FieldTags are tautological because they only re-assert values
set in struct literals; replace these with reflection-based assertions that
verify the actual struct field tags (e.g., the ovsdb/model tags) on the types
instead of field values. Use reflect.TypeOf(LogicalRouterStaticRoute{}) and
reflect.TypeOf(LogicalRouter{}) to inspect fields UUID, IPPrefix, Nexthop,
ExternalIDs, Policy and UUID, Name, StaticRoutes respectively, and assert that
Tag.Get("ovsdb") (or the correct tag key used by your OVSDB mapping) equals the
expected tag/schema name for each field; update the test expectations to the
correct tag strings so the tests fail on tag/schema regressions. Ensure you keep
the original test names and replace value assertions with these tag checks.
In `@pkg/controllers/c2cc/ovn.go`:
- Around line 25-28: The current buildNamedUUID uses strings.NewReplacer to
sanitize a few characters but misses other characters (e.g., from IPv6 CIDRs);
change buildNamedUUID to produce a URL-safe deterministic token by encoding the
combined prefix+suffix with a safe encoder: e.g., compute a SHA-256 (or other
stable) hash of prefix+suffix and return its URL-safe base64
(base64.RawURLEncoding.EncodeToString(hash[:])) or, if readability is desired,
return url.PathEscape(prefix+suffix) to percent-encode all unsafe characters;
update the buildNamedUUID function to use one of these approaches so all
possible characters are handled safely.
In `@test/resources/c2cc.resource`:
- Around line 174-180: Replace the fragile Python inline in the "Verify Node
SNAT Annotation" check: instead of piping oc JSON to python3 -c, call Oc On
Cluster to run oc get node with a jsonpath that directly extracts the annotation
key 'k8s.ovn.org/node-ingress-snat-exclude-subnets' (for example using oc get
node -o
jsonpath="{.items[0].metadata.annotations['k8s.ovn.org/node-ingress-snat-exclude-subnets']}")
so ${stdout} contains the annotation value reliably; update the command that
currently uses python3 -c in the test to use this jsonpath form and keep the
subsequent Should Contain assertions unchanged.
In `@test/suites/c2cc/reconciliation.robot`:
- Around line 19-21: The test's reconciliation timeout (${RECONCILE_TIMEOUT}) is
likely too short for slower environments; increase ${RECONCILE_TIMEOUT} (or
adjust ${RECONCILE_RETRY}) so the suite allows more attempts given the
controller's reconcileInterval (~10s). Update the Variables block to set a
larger ${RECONCILE_TIMEOUT} (for example to 60s or more) or reduce
${RECONCILE_RETRY} gap to ensure enough reconciliation attempts relative to
reconcileInterval and event-driven triggers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 66502359-d58b-4535-b110-837f5171a8be
⛔ Files ignored due to path filters (179)
go.sumis excluded by!**/*.sumvendor/github.com/cenkalti/backoff/v4/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/backoff.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/context.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/exponential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/retry.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/ticker.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/timer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/tries.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/hub/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/hub/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/hub/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/hub/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/hub/hub.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/codec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/debug.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/jsonrpc/jsonrpc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/state.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/.gitattributesis excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/charset/charset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/csv/parser.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/json/parser.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/archive.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/audio.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/binary.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/database.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/document.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/font.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/ftyp.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/geo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/image.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/magic.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/ms_office.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/netpbm.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/ogg.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/text.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/text_csv.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/video.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/zip.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/markup/markup.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/scan/bytes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/mime.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/mimetype.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/supported_mimes.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/tree.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/currency/currency.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/logo.pngis excluded by!**/*.png,!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/rules.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/import_export.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/logo.pngis excluded by!**/*.png,!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/translator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/universal_translator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/MAINTAINERS.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/baked_in.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/cache.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/country_codes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/currency_codes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/field_level.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/logo.pngis excluded by!**/*.png,!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/postcode_regexes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/regexes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/struct_level.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/translations.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/validator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/validator_instance.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/kind.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/machine.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/machine.go.rlis excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/parsing_mode.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/scim.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/scim/schema/type.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/urn.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/urn8141.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/NOTICEis excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/cache/cache.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/cache/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/cache/uuidset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/api.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/api_test_model.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/condition.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/metrics.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/monitor.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/validation.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/database/database.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/database/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/database/references.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/mapper/info.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/mapper/mapper.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/model/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/model/database.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/model/model.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/bindings.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/condition.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/error.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/map.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/monitor_select.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/mutation.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/named_uuid.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/notation.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/row.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/rpc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/schema.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/serverdb/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/serverdb/database.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/serverdb/gen.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/serverdb/model.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/set.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/update3.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/updates.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/updates2.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/uuid.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/updates/difference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/updates/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/updates/merge.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/updates/mutate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/updates/references.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/updates/updates.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/sha3/hashes.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/sha3/legacy_hash.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/sha3/legacy_keccakf.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/sha3/shake.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/Makefileis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/README.mdis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/SECURITY_CONTACTSis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/code-of-conduct.mdis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/error.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/exec.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/fake.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/nftables.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/objects.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/transaction.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/util.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (29)
go.modpkg/cmd/run.gopkg/config/c2cc.gopkg/controllers/c2cc/annotation.gopkg/controllers/c2cc/annotation_test.gopkg/controllers/c2cc/controller.gopkg/controllers/c2cc/helpers_test.gopkg/controllers/c2cc/nbdb.gopkg/controllers/c2cc/nbdb_test.gopkg/controllers/c2cc/networkpolicy.gopkg/controllers/c2cc/nftables.gopkg/controllers/c2cc/nftables_test.gopkg/controllers/c2cc/ovn.gopkg/controllers/c2cc/ovn_test.gopkg/controllers/c2cc/policy_routes.gopkg/controllers/c2cc/policy_routes_test.gopkg/controllers/c2cc/routes.gopkg/controllers/c2cc/routes_test.gopkg/controllers/c2cc/service_routes.gopkg/controllers/c2cc/service_routes_test.gotest/assets/c2cc/curl-pod.yamltest/assets/c2cc/hello-microshift.yamltest/bin/scenario.shtest/resources/c2cc.resourcetest/scenarios-bootc/el9/presubmits/el98-src@c2cc.shtest/suites/c2cc/connectivity.robottest/suites/c2cc/infrastructure.robottest/suites/c2cc/reconciliation.robottest/suites/c2cc/sanity.robot
Member
Author
|
/test ? |
Member
Author
|
/test pull-ci-openshift-microshift-main-e2e-aws-tests-bootc-el9 |
Member
Author
|
/test e2e-aws-tests-bootc-arm-el9 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/controllers/c2cc/nftables.go (1)
187-201: Use a resettable timer for the debounce loop.
time.Aftercreates a new timer for every event and can't be canceled when a newer event arrives. A reusabletime.Timerwould cut timer churn and make shutdown cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/c2cc/nftables.go` around lines 187 - 201, Replace the time.After-based debounce in the goroutine that watches rawCh/reconcileCh with a reusable time.Timer: create a timer variable (e.g., debounceTimer *time.Timer) initially nil, on receiving a rawCh event start the timer if nil (time.NewTimer(2*time.Second)) or call debounceTimer.Reset(2*time.Second) if already exists, on the timer channel fire send the "nftables-change" to reconcileCh, and on goroutine shutdown stop the timer and drain its channel if needed (use debounceTimer.Stop() and drain to avoid races). Ensure you reference the existing rawCh and reconcileCh identifiers and replace uses of the local debounce <-chan time.Time with this resettable timer logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/c2cc/nftables.go`:
- Around line 164-167: The current handler tears down the nftables watcher by
returning on a single sock.Receive() error; change it to keep the watcher alive
by replacing the immediate return with a retry/backoff loop that either
continues on transient errors or attempts to recreate the netlink socket (the
sock used for sock.Receive()) and resume receiving; update the logic around
msgs, _, err := sock.Receive() so errors are logged, waited-on with exponential
backoff, and the socket is reinitialized when necessary instead of returning,
ensuring the watcher goroutine does not exit on the first read error.
In `@pkg/controllers/c2cc/policy_routes.go`:
- Around line 67-98: cleanupRoutes and cleanupRules currently swallow deletion
errors (they log but always return nil); change both functions to collect errors
from netlink.RouteDel and netlink.RuleDel (e.g., store the first error or
aggregate multiple errors) and return a non-nil error when any deletion fails.
Specifically, in cleanupRoutes iterate over routes and on netlink.RouteDel
append or wrap the error (include context like route.Dst, t.table) instead of
only klog.Errorf, and similarly in cleanupRules capture errors from
netlink.RuleDel for rules matching t.priority and t.table and return an
aggregated/wrapped error at the end so callers see cleanup failures.
---
Nitpick comments:
In `@pkg/controllers/c2cc/nftables.go`:
- Around line 187-201: Replace the time.After-based debounce in the goroutine
that watches rawCh/reconcileCh with a reusable time.Timer: create a timer
variable (e.g., debounceTimer *time.Timer) initially nil, on receiving a rawCh
event start the timer if nil (time.NewTimer(2*time.Second)) or call
debounceTimer.Reset(2*time.Second) if already exists, on the timer channel fire
send the "nftables-change" to reconcileCh, and on goroutine shutdown stop the
timer and drain its channel if needed (use debounceTimer.Stop() and drain to
avoid races). Ensure you reference the existing rawCh and reconcileCh
identifiers and replace uses of the local debounce <-chan time.Time with this
resettable timer logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ca3c2996-c214-452b-92cd-c6c0af3d6b18
📒 Files selected for processing (5)
pkg/controllers/c2cc/annotation.gopkg/controllers/c2cc/nbdb.gopkg/controllers/c2cc/nftables.gopkg/controllers/c2cc/policy_routes.gopkg/controllers/c2cc/service_routes.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controllers/c2cc/annotation.go
- pkg/controllers/c2cc/service_routes.go
Member
Author
|
/test e2e-aws-tests-bootc-arm-el9 |
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
/test e2e-aws-tests-bootc-arm-el9 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/config/c2cc.go (1)
52-59: UseResolvedfor the annotation CIDR list.Line 52 flattens
RemoteClusters, butpkg/controllers/c2cc/controller.go:136-159feeds every other reconciler fromc.Resolved. Inputs like10.45.1.7/16get normalized for routes/rules while this annotation still publishes the original string, so the annotation can drift from what the controller actually programs.Suggested change
func (c *C2CC) AllRemoteCIDRs() []string { var cidrs []string - for _, rc := range c.RemoteClusters { - cidrs = append(cidrs, rc.ClusterNetwork...) - cidrs = append(cidrs, rc.ServiceNetwork...) + for _, rc := range c.Resolved { + for _, cidr := range rc.ClusterNetwork { + cidrs = append(cidrs, cidr.String()) + } + for _, cidr := range rc.ServiceNetwork { + cidrs = append(cidrs, cidr.String()) + } } return cidrs }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/c2cc.go` around lines 52 - 59, The AllRemoteCIDRs method currently flattens c.RemoteClusters (using rc.ClusterNetwork/ServiceNetwork) which can differ from the normalized CIDRs the controller actually programs; update AllRemoteCIDRs to iterate over c.Resolved (the normalized/resolved CIDR entries) and return those strings instead so the annotation matches what the reconciler uses (refer to C2CC.AllRemoteCIDRs and the c.Resolved field used by pkg/controllers/c2cc/controller.go).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/c2cc/controller.go`:
- Around line 161-163: initForCleanup must not ignore errors from
initKubeClient; capture the returned error from initKubeClient() inside
C2CCRouteManager.initForCleanup, and if it fails log the error (use the
controller's logger) and return a safe no-op cleanup function that does not
assume c.annotation or c.netpol were set; only set c.annotation and c.netpol
when initKubeClient succeeds so cleanupAll() is only used when kube setup
completed. Ensure you reference initForCleanup, initKubeClient, c.annotation,
c.netpol and cleanupAll when making the change.
In `@pkg/controllers/c2cc/service_routes.go`:
- Around line 125-150: reconcileRules() currently logs failures from
netlink.RuleAdd and netlink.RuleDel but always returns nil; collect these
failures into an errors slice (e.g., errs := []error{}) when netlink.RuleAdd
returns a non-syscall.EEXIST error and when netlink.RuleDel returns an error,
append the errors and at the end return errors.Join(errs...) (matching the
pattern used in routes.go). Keep existing behavior of skipping EEXIST in the
RuleAdd branch and preserve logging, but ensure the function returns the
aggregated error instead of nil so callers see reconciliation failures;
reference symbols: reconcileRules, netlink.RuleAdd, netlink.RuleDel, ruleKey,
actualKeys, desiredKeys.
In `@test/resources/c2cc.resource`:
- Around line 136-154: The tests Verify IP Rules For Table 200 and Verify
Service IP Rules should assert the controller-managed rule priorities instead of
only checking lookup tables; update the assertions to require the priority
number emitted by `ip rule show` (controller uses priority 100 in
pkg/controllers/c2cc/routes.go for lookup 200 and priority 99 in
pkg/controllers/c2cc/service_routes.go for lookup 201), i.e., ensure the lines
include the priority (100 for rules pointing to lookup 200 and 99 for rules
pointing to lookup 201) together with the CIDR and lookup table in the Should
Contain checks.
- Around line 99-113: The helper "Oc On Cluster" creates a temp file via Create
Random Temp File and never deletes it; after calling Process.Run Process and
reading the file into ${stdout}, call the OperatingSystem keyword Remove File
(Remove File ${stdout_file}) before RETURN to delete the temp file, and also
add a best-effort cleanup immediately after the rc assertion (e.g., Run Keyword
And Ignore Error Remove File ${stdout_file}) so the temp file is removed
even if the test fails; references: Oc On Cluster, ${stdout_file}, Create Random
Temp File, Process.Run Process, Remove File, Run Keyword And Ignore Error.
---
Nitpick comments:
In `@pkg/config/c2cc.go`:
- Around line 52-59: The AllRemoteCIDRs method currently flattens
c.RemoteClusters (using rc.ClusterNetwork/ServiceNetwork) which can differ from
the normalized CIDRs the controller actually programs; update AllRemoteCIDRs to
iterate over c.Resolved (the normalized/resolved CIDR entries) and return those
strings instead so the annotation matches what the reconciler uses (refer to
C2CC.AllRemoteCIDRs and the c.Resolved field used by
pkg/controllers/c2cc/controller.go).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ad57e56b-d355-47be-968e-7a293752d024
⛔ Files ignored due to path filters (147)
go.sumis excluded by!**/*.sumvendor/github.com/cenkalti/backoff/v4/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/backoff.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/context.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/exponential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/retry.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/ticker.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/timer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/tries.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/hub/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/hub/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/hub/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/hub/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/hub/hub.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/codec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/debug.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/jsonrpc/jsonrpc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/state.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/.gitattributesis excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/charset/charset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/csv/parser.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/json/parser.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/archive.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/audio.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/binary.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/database.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/document.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/font.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/ftyp.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/geo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/image.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/magic.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/ms_office.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/netpbm.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/ogg.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/text.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/text_csv.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/video.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/zip.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/markup/markup.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/scan/bytes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/mime.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/mimetype.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/supported_mimes.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/tree.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/currency/currency.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/logo.pngis excluded by!**/*.png,!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/rules.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/import_export.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/logo.pngis excluded by!**/*.png,!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/translator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/universal_translator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/MAINTAINERS.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/baked_in.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/cache.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/country_codes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/currency_codes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/field_level.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/logo.pngis excluded by!**/*.png,!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/postcode_regexes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/regexes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/struct_level.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/translations.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/validator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/validator_instance.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/kind.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/machine.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/machine.go.rlis excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/parsing_mode.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/scim.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/scim/schema/type.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/urn.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/urn8141.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/NOTICEis excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/cache/cache.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/cache/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/cache/uuidset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/api.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/api_test_model.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/condition.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/metrics.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/monitor.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/validation.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/database/database.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/database/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/database/references.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/mapper/info.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/mapper/mapper.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/model/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/model/database.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/model/model.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/bindings.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/condition.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/error.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/map.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/monitor_select.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/mutation.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/named_uuid.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/notation.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/row.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/rpc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/schema.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/serverdb/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/serverdb/database.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/serverdb/gen.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/serverdb/model.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/set.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (29)
go.modpkg/cmd/run.gopkg/config/c2cc.gopkg/controllers/c2cc/annotation.gopkg/controllers/c2cc/annotation_test.gopkg/controllers/c2cc/controller.gopkg/controllers/c2cc/helpers_test.gopkg/controllers/c2cc/nbdb.gopkg/controllers/c2cc/nbdb_test.gopkg/controllers/c2cc/networkpolicy.gopkg/controllers/c2cc/nftables.gopkg/controllers/c2cc/nftables_test.gopkg/controllers/c2cc/ovn.gopkg/controllers/c2cc/ovn_test.gopkg/controllers/c2cc/policy_routes.gopkg/controllers/c2cc/policy_routes_test.gopkg/controllers/c2cc/routes.gopkg/controllers/c2cc/routes_test.gopkg/controllers/c2cc/service_routes.gopkg/controllers/c2cc/service_routes_test.gotest/assets/c2cc/curl-pod.yamltest/assets/c2cc/hello-microshift.yamltest/bin/scenario.shtest/resources/c2cc.resourcetest/scenarios-bootc/el9/presubmits/el98-src@c2cc.shtest/suites/c2cc/connectivity.robottest/suites/c2cc/infrastructure.robottest/suites/c2cc/reconciliation.robottest/suites/c2cc/sanity.robot
✅ Files skipped from review due to trivial changes (3)
- test/assets/c2cc/curl-pod.yaml
- test/assets/c2cc/hello-microshift.yaml
- pkg/controllers/c2cc/helpers_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- pkg/cmd/run.go
- pkg/controllers/c2cc/annotation_test.go
- pkg/controllers/c2cc/policy_routes_test.go
- pkg/controllers/c2cc/networkpolicy.go
- pkg/controllers/c2cc/routes_test.go
- test/suites/c2cc/connectivity.robot
- pkg/controllers/c2cc/service_routes_test.go
- pkg/controllers/c2cc/nbdb_test.go
- pkg/controllers/c2cc/policy_routes.go
- test/scenarios-bootc/el9/presubmits/el98-src@c2cc.sh
- pkg/controllers/c2cc/ovn.go
- pkg/controllers/c2cc/nftables_test.go
agullon
reviewed
Apr 29, 2026
| CLUSTER_B_SVC_CIDR="10.46.0.0/16" | ||
| CLUSTER_B_DOMAIN="cluster-b.remote" | ||
|
|
||
| configure_c2cc_hosts() { |
Contributor
There was a problem hiding this comment.
every line in this func is duplicated for both host.
can we run it twice with hostA and hostB params? or should this done in a particular order? because it waits for greenboot and restart microshift I guess not, but it feels weird having all lines duplicated twice
plus: are we planning testing with 3 VMs in the future?
Member
Author
There was a problem hiding this comment.
addressed the duplication
as for 3 VMs: potentially yes, maybe for GA? didn't have specific plans for it yet
The node watch was triggering a full reconcile on every node modification (scheduling, status, labels). Now it extracts the node object from the watch event and only signals when the SNAT annotation actually differs from the desired value. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The debounce goroutine relied on rawCh being closed (via sock.Close -> Receive error -> reader return) to exit. Now it selects on ctx.Done() directly, so shutdown is explicit and doesn't depend on the error-handling cascade. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When multiple events queue in the buffered reconcileCh, each one was triggering a separate full reconcile. Now the loop drains all pending events before running one reconcile, reducing redundant work under churn. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
agullon
reviewed
Apr 29, 2026
agullon
reviewed
Apr 30, 2026
agullon
reviewed
Apr 30, 2026
agullon
reviewed
Apr 30, 2026
agullon
reviewed
Apr 30, 2026
Contributor
|
I reviewed |
- Log warning when kube client init fails during cleanup - Propagate service rule reconciliation failures (matching routes.go pattern) - Replace inline Python with jsonpath in SNAT annotation test Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The annotation manager now treats k8s.ovn.org/node-ingress-snat-exclude-subnets as shared state. A tracking annotation (microshift.io/c2cc-snat-subnets) records which CIDRs C2CC last wrote, so reconcile merges only our entries and cleanup removes only our entries, preserving subnets added by other components. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds a cleanup.robot suite that disables C2CC by moving the config drop-in aside, restarts MicroShift, and verifies all networking state (routes, rules, nftables, OVN routes, annotations, NetworkPolicy) is removed. Teardown restores the config so subsequent suites are unaffected. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/c2cc/controller.go`:
- Around line 100-103: The ready channel is closed before any reconciliation
runs, so move the readiness signal until after the first reconciliation
completes: call c.fullReconcile(ctx) first, wait for it to return
(handle/propagate or log any error), and only then call close(ready) and
klog.Infof("Ready..."); update the sequence around the ready variable and the
c.fullReconcile(ctx) invocation in controller.go so dependents see the service
as ready only after the initial routes/rules/nftables/NetworkPolicy have been
applied.
In `@pkg/controllers/c2cc/ovn.go`:
- Around line 186-189: The cleanup code currently only checks the
transport-level error from m.nbClient.Transact(ctx, ops...) and ignores
per-operation OVSDB errors; update the cleanup path (same place that handles
route removal around route.UUID) to inspect the returned transaction result for
per-op errors (like reconcile() does), log any operation-level failures with
klog.Errorf including route.UUID and the op error details, and append those
errors to errs so stale routes are not treated as successfully removed.
In `@test/scenarios-bootc/el9/presubmits/el98-src`@c2cc.sh:
- Around line 98-106: Replace the direct scp call with the existing
copy_file_from_vm helper so IPv6 wrapping is preserved: after
wait_for_microshift_to_be_ready(host2) keep the run_command_on_vm host2 that
creates and chmods /tmp/kubeconfig-b using ${host2_ip}, then call
copy_file_from_vm host2 "/tmp/kubeconfig-b" "${kubeconfig_b}" instead of scp -P
"${host2_ssh_port}" ...; remove the scp invocation and any manual use of
host2_ssh_port/host2_ip for fetching the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 731ce12b-f90a-4ba9-858e-ae6e502c9e7a
📒 Files selected for processing (18)
pkg/controllers/c2cc/annotation.gopkg/controllers/c2cc/annotation_test.gopkg/controllers/c2cc/controller.gopkg/controllers/c2cc/nbdb.gopkg/controllers/c2cc/networkpolicy.gopkg/controllers/c2cc/nftables.gopkg/controllers/c2cc/ovn.gopkg/controllers/c2cc/policy_routes.gopkg/controllers/c2cc/routes.gopkg/controllers/c2cc/service_routes.gotest/bin/scenario.shtest/resources/c2cc.resourcetest/scenarios-bootc/el9/presubmits/el98-src@c2cc.shtest/suites/c2cc/cleanup.robottest/suites/c2cc/connectivity.robottest/suites/c2cc/infrastructure.robottest/suites/c2cc/reconciliation.robottest/suites/c2cc/sanity.robot
✅ Files skipped from review due to trivial changes (2)
- test/suites/c2cc/connectivity.robot
- pkg/controllers/c2cc/policy_routes.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/controllers/c2cc/networkpolicy.go
- pkg/controllers/c2cc/nbdb.go
- pkg/controllers/c2cc/service_routes.go
- pkg/controllers/c2cc/routes.go
- test/resources/c2cc.resource
The cleanup path only checked transport-level errors from Transact(), silently ignoring per-operation OVSDB errors. Mirror the reconcile path by inspecting each result for operation-level failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace raw scp with copy_file_from_vm helper which handles IPv6 address wrapping, preparing for future dual-stack test scenarios. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/scenarios-bootc/el9/presubmits/el98-src`@c2cc.sh:
- Around line 58-63: The calls to wait_for_greenboot_on_hosts and
wait_for_microshift_to_be_ready currently rely on global errexit; change each
invocation (e.g. wait_for_greenboot_on_hosts "c2cc_pre_greenboot",
wait_for_greenboot_on_hosts "c2cc_greenboot", and
wait_for_microshift_to_be_ready) to perform an explicit return-code check: run
the function, test its exit status with if ! <call>; then emit a clear error
message (e.g. via echo or logger) and exit or return a non-zero status (exit 1)
to fail fast; keep configure_c2cc_host calls unchanged but ensure subsequent
waits follow this explicit-check pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c46ac14a-0f05-4c99-b70d-121c97d50990
📒 Files selected for processing (2)
pkg/controllers/c2cc/ovn.gotest/scenarios-bootc/el9/presubmits/el98-src@c2cc.sh
✅ Files skipped from review due to trivial changes (1)
- pkg/controllers/c2cc/ovn.go
Contributor
|
@pmtk: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/controllers/c2cc/ovn.go (1)
230-252: 💤 Low valueConsider using a backoff for reconnect polling.
The 1-second polling loop at lines 238-244 is functional but will generate constant CPU wake-ups during prolonged outages. An exponential backoff would be gentler, though not critical for MicroShift's typical deployment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/c2cc/ovn.go` around lines 230 - 252, The reconnect loop spawned in the goroutine using m.nbClient.DisconnectNotify() currently polls m.nbClient.Connected() every 1s; replace that tight fixed-interval loop with an exponential backoff retry (e.g., start at ~1s, double on each failed try up to a max like 30s) and reset the backoff when Connected() returns true. Update the inner select that checks ctx.Done() and the time.After to use the backoff duration, and keep sending "ovn-reconnected" on reconcileCh as before; reference the goroutine, m.nbClient.DisconnectNotify(), m.nbClient.Connected(), ctx.Done(), and reconcileCh when making the change.test/resources/c2cc.resource (1)
205-217: 💤 Low valueInline Python JSON manipulation could fail silently on malformed input.
If
${current}is empty or malformed JSON, the Python one-liner will raise an exception that may produce unclear test failures. Consider adding a sanity check or fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/resources/c2cc.resource` around lines 205 - 217, The inline Python one-liner that builds ${new_value} from ${current} can fail if ${current} is empty or malformed; update the Command On Cluster invocation that generates ${new_value} so the Python snippet defensively handles bad input (check if stdin is empty or not valid JSON and default to an empty list), then append ${foreign_cidr} and print the JSON-sorted result; reference the variables and keywords: Get Node SNAT Annotation (to obtain ${current}), ${current}, ${new_value}, and the python3 one-liner used in Command On Cluster, and ensure the patched oc annotate step still receives the JSON string output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/controllers/c2cc/ovn.go`:
- Around line 230-252: The reconnect loop spawned in the goroutine using
m.nbClient.DisconnectNotify() currently polls m.nbClient.Connected() every 1s;
replace that tight fixed-interval loop with an exponential backoff retry (e.g.,
start at ~1s, double on each failed try up to a max like 30s) and reset the
backoff when Connected() returns true. Update the inner select that checks
ctx.Done() and the time.After to use the backoff duration, and keep sending
"ovn-reconnected" on reconcileCh as before; reference the goroutine,
m.nbClient.DisconnectNotify(), m.nbClient.Connected(), ctx.Done(), and
reconcileCh when making the change.
In `@test/resources/c2cc.resource`:
- Around line 205-217: The inline Python one-liner that builds ${new_value} from
${current} can fail if ${current} is empty or malformed; update the Command On
Cluster invocation that generates ${new_value} so the Python snippet defensively
handles bad input (check if stdin is empty or not valid JSON and default to an
empty list), then append ${foreign_cidr} and print the JSON-sorted result;
reference the variables and keywords: Get Node SNAT Annotation (to obtain
${current}), ${current}, ${new_value}, and the python3 one-liner used in Command
On Cluster, and ensure the patched oc annotate step still receives the JSON
string output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e8939fdf-a12c-4c50-8eba-742f89423f79
📒 Files selected for processing (4)
pkg/controllers/c2cc/ovn.gotest/resources/c2cc.resourcetest/scenarios-bootc/el9/presubmits/el98-src@c2cc.shtest/suites/c2cc/cleanup.robot
🚧 Files skipped from review as they are similar to previous changes (2)
- test/suites/c2cc/cleanup.robot
- test/scenarios-bootc/el9/presubmits/el98-src@c2cc.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When reviewing, skip
Vendor libovsdb & knftablescommitSummary by CodeRabbit
New Features
Tests